-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rate limit inttests #347
Rate limit inttests #347
Conversation
setup/hermes/config.toml
Outdated
@@ -102,7 +102,7 @@ store_prefix = 'ibc' | |||
default_gas = 100000 | |||
max_gas = 3000000 | |||
gas_price = { price = 0.0025, denom = 'untrn' } | |||
gas_multiplier = 2.0 | |||
gas_multiplier = 2.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? because of computation increase during packet_recv and packet_send handlers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, otherwise during ibc_transfer
test contract isn't refunded because of out of gas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's got required to increase the multiplier because of a poor simulation process or because the IBC transactions reached the ceiling of max_gas = 3000000
. if the latter, why don't increase the max_gas
instead of the multiplier? would be cleaner
19ece40
to
a6298fc
Compare
a6298fc
to
2ebf08f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great to see the following additional test scenarios:
- quota reset, i.e. IBC transfer limits are reset and transfers work again after a quota period is renewed
- multiple quotas of different limits and time spans, i.e. if you have a daily and a weekly quotas of 5 and 10 respectfully, even if a daily quota is reached twice and then renewed, the weekly quota should still prevent transfers
- if the quota is reached by a sender, other senders also cannot make more sends
- IBC transfers made by smart contracts via the ibc-transfer module are also limited
This is a very good point, it should be implemented 100% But i don't think we need to implement others if they are covered in unit-tests in Neutron itself |
# Conflicts: # package.json # yarn.lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
const UATOM_IBC_TO_NEUTRON_DENOM = | ||
'ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2'; | ||
|
||
// These are th |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are th?
…umption (by removing osmo specific code)
This is very basic tests for Ibc rate limit module to ensure it works as expected in our environment
neutronjsplus PR